Update TIME and DATE functions #134

MitchellGale · 2022-10-13T18:01:20Z

Description

Add option to accept datetime like string in both TIME and DATE (eg. accept "1999-01-02 12:12:12" for both TIME and DATE.

Strict check on date for testing for valid dates (eg. Don't accept Feb 30th as a valid date)

opensearchsql> SELECT TIME("1999-02-30 21:07:32");                                                                                                                                                                                                          
TransportError(500, 'SemanticCheckException', {'error': {'type': 'SemanticCheckException', 'reason': 'Invalid Query', 'details': 'time:1999-02-30 21:07:32 in unsupported format, please use HH:mm:ss[.SSSSSSSSS]'}, 'status': 400})


opensearchsql> SELECT DATE("1999-02-19 21:07:32");                                                                                                                                                                                                          
fetched rows / total rows = 1/1
+-------------------------------+
| DATE("1999-02-19 21:07:32")   |
|-------------------------------|
| 1999-02-19                    |
+-------------------------------+
opensearchsql> SELECT DATE("1999-02-19");                                                                                                                                                                                                                   
fetched rows / total rows = 1/1
+----------------------+
| DATE("1999-02-19")   |
|----------------------|
| 1999-02-19           |
+----------------------+
opensearchsql> SELECT TIME("1999-02-19 21:07:32");                                                                                                                                                                                                          
fetched rows / total rows = 1/1
+-------------------------------+
| TIME("1999-02-19 21:07:32")   |
|-------------------------------|
| 21:07:32                      |
+-------------------------------+
opensearchsql> SELECT TIME("21:07:32");                                                                                                                                                                                                                     
fetched rows / total rows = 1/1
+--------------------+
| TIME("21:07:32")   |
|--------------------|
| 21:07:32           |
+--------------------+

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…y to parse time out of datetime or date out of datetime. Signed-off-by: MitchellGale-BitQuill <[email protected]>

codecov · 2022-10-13T18:13:20Z

Codecov Report

❗ No coverage uploaded for pull request base (Integ-updateDateFunction@b30d156). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##             Integ-updateDateFunction     #134   +/-   ##
===========================================================
  Coverage                            ?   95.10%           
  Complexity                          ?     3073           
===========================================================
  Files                               ?      303           
  Lines                               ?     8254           
  Branches                            ?      610           
===========================================================
  Hits                                ?     7850           
  Misses                              ?      350           
  Partials                            ?       54

Flag	Coverage Δ
query-workbench	`62.76% <0.00%> (?)`
sql-engine	`97.90% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Yury-Fridlyand

mysql> select DATE(TIME('13:49:00')), TIME(DATE('2020-08-26'));

+------------------------+--------------------------+
| DATE(TIME('13:49:00')) | TIME(DATE('2020-08-26')) |
+------------------------+--------------------------+
| 2022-10-13             | 00:00:00                 |
+------------------------+--------------------------+
1 row in set (0.00 sec)

opensearchsql> select DATE(TIME('13:49:00')), TIME(DATE('2020-08-26'));
{'reason': 'Invalid SQL query', 'details': 'date function expected {[STRING],[DATE],[DATETIME],[TIMESTAMP]}, but get [TIME]', 'type': 'ExpressionEvaluationException'}

Yury-Fridlyand · 2022-10-13T19:36:30Z

I think you need to apply/cherry-pick 40ffca2 into your branch to fix that.

…orresponding tests. Signed-off-by: Yury-Fridlyand <[email protected]>

…ction.java Signed-off-by: MitchellGale-BitQuill <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto · 2022-10-14T18:20:04Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -651,7 +652,11 @@ private ExprValue exprConvertTZ(ExprValue startingDateTime, ExprValue fromTz, Ex
   */
  private ExprValue exprDate(ExprValue exprValue) {
    if (exprValue instanceof ExprStringValue) {
-      return new ExprDateValue(exprValue.stringValue());
+      try {


We should be able to remove the if statement and get the same result. The try...catch will cover both cases.

I think we still need it because it can be a stringValue or a dateValue.

is there a case where an ExprStringValue doesn't have a stringValue()?

acarbonetto · 2022-10-14T18:20:30Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -944,6 +949,7 @@ private ExprValue exprSubDateInterval(ExprValue date, ExprValue expr) {
   */
  private ExprValue exprTime(ExprValue exprValue) {
    if (exprValue instanceof ExprStringValue) {
+      //try catch


acarbonetto · 2022-10-14T18:21:26Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

@@ -240,6 +240,11 @@ public void date() {
    assertEquals(DATE, expr.type());
    assertEquals(new ExprDateValue("2020-08-17"), eval(expr));
    assertEquals("date(DATE '2020-08-17')", expr.toString());
+
+    expr = dsl.date(DSL.literal(new ExprDateValue("2020-08-17 12:12:00")));


we should test with nano seconds too

acarbonetto · 2022-10-14T18:21:45Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

@@ -795,6 +800,11 @@ public void time() {
    assertEquals(TIME, expr.type());
    assertEquals(new ExprTimeValue("01:01:01"), eval(expr));
    assertEquals("time(TIME '01:01:01')", expr.toString());
+
+    expr = dsl.time(DSL.literal(new ExprTimeValue("2020-01-02 01:01:01")));


we should test with nano seconds too (separately)

Added case for times < 1.0 seconds. 2016222

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand · 2022-10-19T18:58:09Z

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java

+
+  public static final DateTimeFormatter TIME_FORMATTER_VARIABLE_NANOS_OPTIONAL =
+      new DateTimeFormatterBuilder()
+          .appendPattern("[uuuu-MM-dd HH:mm:ss][HH:mm:ss]")


Do you have tests for both patterns?

All patterns should have coverage

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

Signed-off-by: MitchellGale-BitQuill <[email protected]>

…E functions. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Signed-off-by: MitchellGale-BitQuill <[email protected]>

acarbonetto · 2022-10-20T20:26:34Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -650,8 +652,15 @@ private ExprValue exprConvertTZ(ExprValue startingDateTime, ExprValue fromTz, Ex
   * @return ExprValue.
   */
  private ExprValue exprDate(ExprValue exprValue) {
+    if (exprValue.type() == TIME) {


why not use a switch on type() here. For TIME, STRING, DATE, and TIMESTAMP?

acarbonetto · 2022-10-20T20:27:00Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

    if (exprValue instanceof ExprStringValue) {
-      return new ExprTimeValue(exprValue.stringValue());
+      try {
+        return new ExprTimeValue(exprValue.stringValue());


why not use a switch on type() here. For TIME, STRING, DATE, and TIMESTAMP?

Yury-Fridlyand · 2022-10-25T15:50:51Z

@MitchellGale-BitQuill,
can you add support for TIME(HH:mm)? Current format requires seconds as a mandatory part in input string.

…cal time now. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Signed-off-by: MitchellGale-BitQuill <[email protected]>

MitchellGale · 2022-10-27T18:15:15Z

@Yury-Fridlyand Added support for HH:mm in 61289bb.

MitchellGale · 2022-10-27T19:03:04Z

@[MitchellGale-BitQuill](https://github.com/MitchellGale-BitQuill) MitchellGale-BitQuill changed the title [BLOCKED] Update TIME and DATE functions Update TIME and DATE functions [now](https://github.com/Bit-Quill/opensearch-project-sql/pull/134#event-7685087474)

Removed blocking component. It can be fixed in a later PR.

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand · 2022-10-28T18:25:25Z

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

@@ -67,7 +69,7 @@ public LocalDateTime datetimeValue() {

  @Override
  public Instant timestampValue() {
-    return ZonedDateTime.of(date, timeValue(), ZoneId.systemDefault()).toInstant();
+    return ZonedDateTime.of(date, timeValue(), ZoneId.of("UTC")).toInstant();


Last minor change - you need to revert this as well ... or ... update test to use UTC too.

Signed-off-by: MitchellGale-BitQuill <[email protected]>

…prDateValue.java for timestampValue to use systemDefault instead of UTC time. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Signed-off-by: Andriy Redko <[email protected]>

Added test cases and support for strict date validation. Added abilit…

65c34ac

…y to parse time out of datetime or date out of datetime. Signed-off-by: MitchellGale-BitQuill <[email protected]>

MitchellGale changed the title ~~Added test cases and support for strict date validation. Added abilit…~~ Update TIME and DATE functions Oct 13, 2022

Yury-Fridlyand requested review from Yury-Fridlyand, acarbonetto, forestmvey, GabeFernandez310, GumpacG and MaxKsyunz October 13, 2022 18:16

GumpacG approved these changes Oct 13, 2022

View reviewed changes

forestmvey approved these changes Oct 13, 2022

View reviewed changes

Yury-Fridlyand requested changes Oct 13, 2022

View reviewed changes

Update ExprTimeValue by datetimeValue and other interfaces. Add c…

4a4e425

…orresponding tests. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand mentioned this pull request Oct 14, 2022

SQL ingestion and ops - standup opensearch-project/sql#791

Open

Yury-Fridlyand added 2 commits October 14, 2022 10:55

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFun…

da22234

…ction.java Signed-off-by: MitchellGale-BitQuill <[email protected]>

Fix conversion to timestamp.

e421349

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto approved these changes Oct 14, 2022

View reviewed changes

MitchellGale added 5 commits October 17, 2022 09:52

Added try catch to return null for invalid date in DATE

26c1165

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Added null test for TIME and DATE

be3ea22

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Added exprDate to accept time.

4f7edd1

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Accept date time for time and date

a00863c

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Adding coverage

99c98b0

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand reviewed Oct 19, 2022

View reviewed changes

Yury-Fridlyand requested changes Oct 19, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java Outdated Show resolved Hide resolved

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java Outdated Show resolved Hide resolved

MitchellGale added 4 commits October 19, 2022 13:28

Fixed code coverage for time passed to date function case.

f9e38e9

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Adding more test cases for datetime and milliseconds for DATE and TIM…

2016222

…E functions. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Removed unused imports.

4c0928d

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Removed unused DATETIMEFORMATTERS.

95c5e5c

Signed-off-by: MitchellGale-BitQuill <[email protected]>

acarbonetto reviewed Oct 20, 2022

View reviewed changes

Yury-Fridlyand self-requested a review October 20, 2022 22:13

acarbonetto changed the title ~~Update TIME and DATE functions~~ [BLOCKED] Update TIME and DATE functions0 Oct 25, 2022

acarbonetto changed the title ~~[BLOCKED] Update TIME and DATE functions0~~ [BLOCKED] Update TIME and DATE functions Oct 25, 2022

Added support for HH:mm for time/datetime inputs. Removed call for lo…

61289bb

…cal time now. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand approved these changes Oct 26, 2022

View reviewed changes

Yury-Fridlyand self-requested a review October 26, 2022 00:47

Added more doc tests.

310f653

Signed-off-by: MitchellGale-BitQuill <[email protected]>

MitchellGale changed the title ~~[BLOCKED] Update TIME and DATE functions~~ Update TIME and DATE functions Oct 27, 2022

Yury-Fridlyand reviewed Oct 28, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java Outdated Show resolved Hide resolved

Yury-Fridlyand self-requested a review October 28, 2022 01:09

Removed blocks from cherry-picked PR.

484b28c

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand requested changes Oct 28, 2022

View reviewed changes

MitchellGale added 2 commits October 28, 2022 13:23

Reverted change in ExprDateTimeValue

157059f

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Reverted reverted changes in ExprDateTimeValue. Reverted change in Ex…

77bd46e

…prDateValue.java for timestampValue to use systemDefault instead of UTC time. Signed-off-by: MitchellGale-BitQuill <[email protected]>

Yury-Fridlyand approved these changes Oct 28, 2022

View reviewed changes

MitchellGale merged commit bc1507c into Integ-updateDateFunction Oct 31, 2022

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Update to Gradle 7.5.1 (#134)

9b4e9a6

Signed-off-by: Andriy Redko <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TIME and DATE functions #134

Update TIME and DATE functions #134

MitchellGale commented Oct 13, 2022

codecov bot commented Oct 13, 2022 •

edited

Loading

Yury-Fridlyand left a comment

Yury-Fridlyand commented Oct 13, 2022

acarbonetto Oct 14, 2022

MitchellGale Oct 20, 2022

acarbonetto Oct 20, 2022

acarbonetto Oct 14, 2022

acarbonetto Oct 14, 2022

acarbonetto Oct 14, 2022

MitchellGale Oct 20, 2022

Yury-Fridlyand Oct 19, 2022

MitchellGale Oct 20, 2022

acarbonetto Oct 20, 2022

acarbonetto Oct 20, 2022

Yury-Fridlyand commented Oct 25, 2022

MitchellGale commented Oct 27, 2022

MitchellGale commented Oct 27, 2022

Yury-Fridlyand Oct 28, 2022

Update TIME and DATE functions #134

Update TIME and DATE functions #134

Conversation

MitchellGale commented Oct 13, 2022

Description

Issues Resolved

Check List

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Oct 25, 2022

MitchellGale commented Oct 27, 2022

MitchellGale commented Oct 27, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2022 •

edited

Loading